Allow removal and adding for plugins in iconMenu#861
Conversation
oeninghe-dataport
requested review from
czirkelbach,
dopenguin,
raschju and
warm-coolguy
as code owners
|
| watch(open, (open) => { | ||
| if (open) { | ||
| openInMoveHandle(open) | ||
| } else { | ||
| coreStore.setMoveHandle(null) | ||
| } | ||
| }) |
There was a problem hiding this comment.
I prefer the previously implemented version in favour of this side effect solution. Same goes for focusOpen.
What is the practical reason why this is needed or is simply personal preference?
There was a problem hiding this comment.
Practical reason.
We had that logic already duplicated in NineRegionsButton and StandardMenuList, and I don't wanted to copy it a third time at https://github.com/Dataport/polar/pull/861/changes#diff-8793dc8dda9a07c08b84fcfa5edf13989ab72fde14867879719bfba5765c229aR59-R64
There was a problem hiding this comment.
It could also be de-duplicated by abstracting the toggle functions of both components into an action.
I deem this favourable over the side-effect solution.
| // Otherwise, the component itself is made reactive | ||
| menus.value.map((menuGroup) => | ||
| menuGroup.map((menuItem) => | ||
| toMerged(menuItem, { | ||
| plugin: { | ||
| component: markRaw(menuItem.plugin.component as Component), | ||
| }, | ||
| }) | ||
| ) | ||
| ) | ||
| focusMenus.value.map((menuItem) => | ||
| toMerged(menuItem, { | ||
| plugin: { | ||
| component: markRaw(menuItem.plugin.component as Component), | ||
| }, | ||
| }) | ||
| ) | ||
|
|
There was a problem hiding this comment.
The important part - making the component non-reactive with markRaw - seems to be lost on the new implementation.
There was a problem hiding this comment.
These lines have no effect anymore. Or do I miss anything?
| watch( | ||
| flatMenuItems, | ||
| (newItems, oldItems) => { | ||
| const getDiffItems = (to, from) => | ||
| to.filter( | ||
| (item) => | ||
| !from.some((oldItem) => oldItem.plugin.id === item.plugin.id) | ||
| ) | ||
| getDiffItems(newItems, oldItems).forEach((newItem) => { | ||
| coreStore.addPlugin(toMerged(newItem.plugin, { independent: false })) | ||
| }) | ||
| getDiffItems(oldItems, newItems).forEach((oldItem) => { | ||
| if (open.value === oldItem.plugin.id) { | ||
| open.value = null | ||
| } | ||
| if (focusOpen.value === oldItem.plugin.id) { | ||
| focusOpen.value = null | ||
| } | ||
| coreStore.removePlugin(oldItem.plugin.id) | ||
| }) | ||
| }, | ||
| { deep: true } | ||
| ) |
There was a problem hiding this comment.
I agree that iconMenu should handle the removal itself so the separation of concerns is not broken.
However, I would've thought adding its own removePlugin action that handles that procedure would be the way to go here. Note that, even though they are currently described otherwise in the export, neither menu nor focusMenus are considered stable API. I've made a note for myself to go over all plugins to adjust the exports to the requirements and mark most as @alpha.
There was a problem hiding this comment.
My first approach was to implement a addPlugin and removePlugin action. However, they get quite complicated, as you have to check which index and subindex to consider, remove an empty array if it is empty; and insertion similar.
If you prefer to have these actions, I can add them. I'd stick to this watcher to keep the more flexible option of direct editing, but we could e.g. mark only the actions as stable to keep menus as unstable API.
2 participants
Summary
Plugins in iconMenu can be added and removed at runtime.
Note:
coreStore.removePlugin(...)is still not supported for plugins within iconMenu. I deem this correct. As iconMenu adds the plugin, it should also be responsible for removal of the plugin. Otherwise, we'd need to breach the separation of core and plugins.Instructions for local reproduction and review
Relevant tickets, issues, et cetera
Fixes #668